Codecov Report

Merging #26242 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #26242      +/-   ##
==========================================
- Coverage   91.97%   91.96%   -0.01%     
==========================================
  Files         175      175              
  Lines       52368    52371       +3     
==========================================
  Hits        48164    48164              
- Misses       4204     4207       +3

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9feb3ad...f89c29c. Read the comment docs.

Codecov Report

Merging #26242 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #26242      +/-   ##
==========================================
+ Coverage   91.97%   91.97%   +<.01%     
==========================================
  Files         175      175              
  Lines       52368    52389      +21     
==========================================
+ Hits        48164    48184      +20     
- Misses       4204     4205       +1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9feb3ad...464fa34. Read the comment docs.

Good question, actually. Yes, there is. It's test_basic_categorical (in the same file). But before I added the new test in this pull request, that was the only test that exercised the code paths of merge_asof for categoricals. I feel there should be more of those... That would be another pull request IMHO. though.

Thanks for clarifying. So this change does introduce unordered Categoricals which might be counter to what merge_asof is supposed to offer (the docstring explicitly states the join field must be ordered).

Do you you see any other instances of that method supporting unordered data? Wondering if we need to further require that Categoricals are ordered to use this method

@WillAyd Indeed, the merge keys should be ordered, otherwise merge_asof() makes no sense. Currently, the merge fails later (in _get_join_indexers()) with a ValueError. But fun fact, there is no test currently in test_merge_asof.py. I've added a test that undordered categorical merge keys raises.

I've also added code to raise a MergeError (instead of a ValueError further down the road). But maybe adding this error check in in _get_merge_keys() is not the right place, because by then, the by= keys have been added to the list of merge keys.

Is there a reliable way to skip over the by= keys in this new test? Or should I put this test in another method? Thanks!

Hmm that's unfortunate. If it makes things too complicated I'm not against looking that in more detail as a follow up if you can open an issue for it

Cool. I've reverted the last change, and added a comment to explain what's happening currently. I've created #26249 for the other cleanups.

thanks @chrish42 nice patch!